[ISSUE #10737] Propagate admin future failures - #10738
Conversation
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot
Summary
Fixes CompletableFuture propagation in all 19 async admin operations of MqClientAdminImpl. The previous thenAccept pattern left the caller's future permanently pending when the remoting call failed or the response handler threw.
Findings
- [Info]
MqClientAdminImpl.java:406-420— The newhandleResponsehelper correctly useswhenCompleteto bridge both remoting failures and handler exceptions to the public future. CatchingThrowablein the handler path is appropriate since decoder/handler code may throw unchecked exceptions. - [Info]
MqClientAdminImpl.java— All 19 call sites are updated consistently with the same pattern. No call site was missed. - [Info]
MqClientAdminImplTest.java:529-596— TheallOperationsShouldPropagateRemotingFailuretest efficiently covers all 19 operations in a single test by collecting futures and asserting after a singlecompleteExceptionally. TheoperationShouldPropagateResponseHandlerFailuretest covers the decoder-failure path with a null body triggering NPE.
Suggestions
No issues found. The fix is minimal, correct, and well-tested.
- Correctness: The root cause (pending futures on error) is properly addressed.
whenCompleteis the right choice overhandleorexceptionallysince it preserves the void-like bridge pattern. - Compatibility: No public API or protocol change. The
handleResponsemethod is private. - Tests: 41/41 tests pass, including the two new regression tests.
LGTM.
Automated review by github-manager-bot
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
Defensive fix with proper validation and test coverage. LGTM.
Automated review by github-manager-bot
Signed-off-by: Rui <[email protected]>
7fa9ca0 to
d11b6f5
Compare
|
Refreshed this PR onto the current
The production change is +35/-19 lines; the remaining +71 lines are focused failure-propagation tests for the async admin methods. Full CI has been retriggered. @drpmma @lizhimins, could you please take a human review when convenient, especially of the shared |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #10738 +/- ##
=============================================
- Coverage 48.58% 48.51% -0.07%
+ Complexity 13678 13655 -23
=============================================
Files 1381 1381
Lines 101475 101485 +10
Branches 13190 13190
=============================================
- Hits 49304 49239 -65
- Misses 46170 46229 +59
- Partials 6001 6017 +16 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
3 participants
Which Issue(s) This PR Fixes
Brief Description
MqClientAdminImplusedthenAcceptto complete a separate result future and ignored the dependent stage. An exceptional remoting completion skipped the handler, while a decoder/handler exception completed only that ignored stage; in both cases the future returned to the caller remained pending.Add one generic response bridge and use it for all 19 asynchronous admin operations. The bridge propagates the upstream remoting throwable and completes the public future exceptionally if existing response handling throws. Normal SUCCESS and non-SUCCESS response-code behavior is unchanged, and there is no public API or protocol change.
How Did You Test This Change?
MqClientAdminImplTest: 41/41 passed.validate/ Checkstyle: 0 violations across common, remoting, and client.BugInstance=0,Error=0across common, remoting, and client.git diff --check: passed.